Fix CNS and CNI ETW log interference issue #3692
Conversation
Signed-off-by: Yerlan Baiturinov <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR resolves ETW session conflicts by giving CNS and CNI separate Geneva profile names for logging.
- Change default ETW provider in
ConfigtoACN-Monitoring-CNS - Update CNS logging core to use
ACN-Monitoring-CNS - Update CNI logging core to use
ACN-Monitoring-CNI
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cns/logger/v2/config_windows.go | Default ETW.ProviderName updated to ACN-Monitoring-CNS |
| cns/logger/cnslogger_windows.go | zapetw.New call now uses ACN-Monitoring-CNS |
| cni/log/logger_windows.go | zapetw.New call now uses ACN-Monitoring-CNI |
Comments suppressed due to low confidence (3)
cns/logger/cnslogger_windows.go:22
- There are no unit tests verifying the ETW core uses the new provider name; consider adding tests to assert the correct Geneva profile is set for CNS logging.
etwcore, _, err := zapetw.New("ACN-Monitoring-CNS", etwCNSEventName, encoder, loggingLevel)
cni/log/logger_windows.go:29
- Similar to CNS, CNI logging should have a test to confirm it picks up the
ACN-Monitoring-CNIprovider name; adding a unit test would prevent regressions.
etwcore, _, err := zapetw.New("ACN-Monitoring-CNI", etwCNIEventName, jsonEncoder, loggingLevel)
cns/logger/v2/config_windows.go:22
- The change to default ETW provider names isn't reflected in comments or external docs; update any related documentation to note the new defaults for CNS and CNI.
func (c *Config) normalize() {
Signed-off-by: Yerlan Baiturinov <[email protected]>
|
I'm not sure that this will necessarily solve the root issue. There can be multiple events published by the same provider. I think the deeper issue is in the current naming convention for event names. While event providers can be hyphenated ("-"), the event name cannot (this isn't listed explicitly in any documentation but is a reinforcement mechanism that Geneva does when parsing the config). This isn't an issue with DNC because having a default event of a different name can find an event name that's hyphenated. For whatever reason, this isn't the case when there are multiple events names from the same provider. With this in mind, I think a more appropriate approach would be to try to change the event names (removing the hyphen in "Azure-CNI") before changing the event provider name. |
Signed-off-by: Yerlan Baiturinov <[email protected]>
|
Let's try just changing the event name at first. If that's sufficient, there's no need to change the provider name. |
Signed-off-by: Yerlan Baiturinov <[email protected]>
Resolved |
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| maxLogFileSizeInMb = 5 | ||
| maxLogFileCount = 8 | ||
| etwCNIEventName = "Azure-CNI" | ||
| etwCNIEventName = "AzureCNI" |
There was a problem hiding this comment.
Curious to know what is this change for?
* fix(log): Fix CNS and CNI ETW log interference issue Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: Docker containers version updated Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: ETW Event Name changed for CNI Signed-off-by: Yerlan Baiturinov <[email protected]> * fix: Revert changing Profilename in the ETW loggers Signed-off-by: Yerlan Baiturinov <[email protected]> --------- Signed-off-by: Yerlan Baiturinov <[email protected]>
5 participants
Reason for Change:
This PR addresses a logging conflict between CNS and CNI when both components attempt to emit ETW logs using the same Geneva profile name. Previously, both services were configured to use ACN-Monitoring, which caused one of the services (typically CNI) to fail to emit logs due to ETW session conflicts.
Changes
Introduced separate Geneva profile names:
Issue Fixed:
Fixes #3691
Requirements:
Notes: